Update driver cache entries to contain the file version #25337
Conversation
github-actions
bot
added
area: driver
There was a problem hiding this comment.
Pull Request Overview
This PR updates driver cache entries to include file version information in the cache key to prevent ops from appearing on incorrect document versions. The change addresses a bug where operations from the main document would incorrectly appear on previous versions.
Key changes:
- Enhanced cache key generation to include file version when available
- Restructured cache key format to better organize version, type, and key components
- Fixed typos in comments and documentation
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/drivers/odsp-driver/src/odspDocumentService.ts | Fixed typo in comment ("re receive" → "we receive") |
| packages/drivers/odsp-driver-definitions/src/odspCache.ts | Updated cache key generation logic to include file version and fixed typo in documentation |
| ? `_${entry.file.resolvedUrl.fileVersion}` | ||
| : ""; | ||
| const suffix = entry.type === snapshotKey ? "" : `_${entry.key}`; | ||
| return `${entry.file.docId}${version}_${entry.type}${suffix}`; |
There was a problem hiding this comment.
This will cause a regression in load performance as we are changing the key corresponding to the stored snapshot. So the previous entries won't be found and instead of loading from cache, we will load from network for each document.
There was a problem hiding this comment.
oh, the version will "" for snapshot key.
There was a problem hiding this comment.
Can you before and after example of how would the key look for snapshot and ops?
There was a problem hiding this comment.
Yes, the goal is to only change the cache entries for versioned snapshots/ops and leave the entries for the main document untouched.
There was a problem hiding this comment.
ok got it. So, when they will start supplying the version, then they will face some regression. You can let them know before hand, when they will start the rollout.
There was a problem hiding this comment.
Examples with versions:
before:
docId_snapshot_4.0
docId_ops_100_2
after:
docId_4.0_snapshot
docId_4.0_ops_100_2
If there is a version value in entry.key for a snapshot entry, it will also be accessible from the resolved url, and will end up in the version variable in the updated function.
| switch (entry.type) { | ||
| case snapshotWithLoadingGroupIdKey: | ||
| case snapshotKey: { | ||
| const version = entry.key === undefined ? "" : `_${entry.key}`; |
There was a problem hiding this comment.
we should get the version from the same place in both paths, and stop passing version as key for snapshots
There was a problem hiding this comment.
only looks like half of this was done. jatin has similar comment which i also replied to about simplifying this function.
by " stop passing version as key for snapshots" i mean the code which calls this function for snapshots shouldn't specify the key field. this function should still encode it so it can be used later if there is a reason to
| return `${entry.file.docId}${version}_${entry.type}_${entry.key}`; | ||
| } | ||
| default: | ||
| return `${entry.file.docId}_${entry.type}_${entry.key}`; |
There was a problem hiding this comment.
Since we should not reach here as there is no other scenario for now. We can use unreachableCase here. Look at unreachableCase at other switch in our code to know more.
| // example versioned entry: docId_4.0_snapshot_ | ||
| // example non-versioned entry: docId_snapshot_ | ||
| // The trailing '_' is included for consistency with existing cache entries | ||
| return `${entry.file.docId}${version}_${entry.type}_`; |
There was a problem hiding this comment.
Maybe be better would be still end this key with entry.key as we should use key but then also set key to "" for now instead of fileVersion as we are getting from resolvedUrl. So, for existing keys it will still remain docId_snapshot_
There was a problem hiding this comment.
This is because as it seems weird to not use the key in creating the final key.
There was a problem hiding this comment.
i think that will also simplify the function, as there won't be a need for special casing based on type
| import { ICacheEntry } from "@fluidframework/odsp-driver-definitions/internal"; | ||
| import { | ||
| ICacheEntry, | ||
| getKeyForCacheEntry as odspGetKeyForCacheEntry, |
There was a problem hiding this comment.
the driver web cache shouldn't depend on the odsp client as it is not odsp specific, but i guess it already did :( we should open an item to clean this up.
There was a problem hiding this comment.
In that case, would it be better to duplicate the implementation here instead of calling the odsp one? just to make the cleanup easier
There was a problem hiding this comment.
for tracking: AB#48070
Previously, only snapshot entries for a given document would contain version information to indicate which version of the document the entry referred to. This caused a bug where ops from the main document would appear on previous versions of the document when they should not yet exist. This PR updates the cache entries of snapshots and ops to both contain the version information in the cache entry, if it exists.
AB#47218
Additionally, fixes a few typos I noticed while working in the area.